Added option for backend to have a vhost override#103
Added option for backend to have a vhost override#103mchandler-plato wants to merge 1 commit intobloomberg:mainfrom
Conversation
Signed-off-by: Mark Chandler <mark@platoteam.com>
adamncasey
left a comment
adamncasey
left a comment
There was a problem hiding this comment.
Thank you for the PR!
The workflow makes sense to me. In the past we had discussed holding a vhostname map, e.g.
amqpprox_ctl /tmp/amqpprox BACKEND ADD backend-1 dc hostname 5672
amqpprox_ctl /tmp/amqpprox BACKEND ADD backend-2 dc hostname-2 5672
amqpprox_ctl /tmp/amqpprox MAP BACKEND old-vhost backend-1
amqpprox_ctl /tmp/amqpprox MAP BACKEND new-vhost backend-2
amqpprox_ctl /tmp/amqpprox VHOST REMAP old-vhost new-vhost
This would remove the need to configure backends twice (e.g. if backend-2 also already mapped a few unrelated vhosts).
What do you think?
| std::string d_datacenterTag; | ||
| std::string d_host; | ||
| std::string d_ip; | ||
| std::string d_virtualHost; |
There was a problem hiding this comment.
I suggest we use std::optional<std::string> here - mainly for the benefit of indicating that it may not be set.
| methods::Open openCopy = d_open; | ||
| openCopy.setVirtualHost(d_sessionState_p->getBackendVirtualHost()); | ||
| sendResponse(openCopy, false); |
There was a problem hiding this comment.
The only property which is encoded/decoded in amqpprox on Open is the vhost so copying the inbound open is a little pointless now, I feel it's neater to keep the Open object immutable and add a constructor which takes a vhost string. I think we could go with something like this, and ditch the d_open property on the Connector
| methods::Open openCopy = d_open; | |
| openCopy.setVirtualHost(d_sessionState_p->getBackendVirtualHost()); | |
| sendResponse(openCopy, false); | |
| methods::Open openMethod{d_sessionState_p->getBackendVirtualHost()}; | |
| sendResponse(openMethod, false); |
There was a problem hiding this comment.
I wasnt sure what else used d_open so making a copy was 'safer'. Open to doing what ever way makes more sense
|
@adamncasey thanks for the review, VHOST remap does sound like a better solution, i take it when we get the open package from the client, we would then look up the mapping and update it? Will have to wait a few weeks till i get some time to try that change |
2 participants
Describe your changes
Allows a backend to specify a different vhost to connect to than the one provided by the client. This allows easy migration of clients to new rabbitmq cluster with a different vhost.
Testing performed
Run setup in production on live rabbitmq cluster with clients using default vhost '/' and redirecting them to other vhosts
Additional context
Prob missing a few things, please let me know what improvements i can make